Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Docs network 2.4 Declarative intent #26954

Closed
wants to merge 19 commits into from

Conversation

gundalow
Copy link
Contributor

@gundalow gundalow commented Jul 18, 2017

SUMMARY

New guide_networking section for detailing non-module features:

  • network_declarative_intent
ISSUE TYPE
  • Docs Pull Request

@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2017

The test ansible-test sanity --test rstcheck failed with the following error:

docs/docsite/rst/network_declarative_intent.rst:118:0: Title underline too short.

click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.4 This issue/PR affects Ansible v2.4 ci_verified Changes made in this PR are causing tests to fail. docs_pull_request needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 18, 2017
Overview
========

Consider the case if you wanted to ensure a set of vlans are present on a switch. Although at a high level thia sounds like a simple request, there in a slight nuance:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thia -> this

Overview
========

Consider the case if you wanted to ensure a set of vlans are present on a switch. Although at a high level thia sounds like a simple request, there in a slight nuance:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thia -> this


Continuing our vlan example, the following task ensure that vlans ``1``, ``2`` and ``3`` are in the states specified in the task, in `addition` to any existing vlans.

This task *will not* change any vlans already configured on the switch, apart from the ones specified in the task.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do additive resources just ignore any existing identical items and mark them as "unchanged"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added

Ansible will ensure that the vlans specified in the task exist with the name and state specified.
Is that clearer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works

vlan_id: “{{ item.vlan_id }}”
name: “{{ item.name }}”
state: “{{ item.state | default(‘active’) }}”
with_items:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason additive items are added using a for loop rather than with an "additive" keyword like aggregate resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. Originally I wanted to show the different ways of doing things, though that may just confuse the matter.

I've added a FIXME in the docs.


This task is very similar to the `additive resource` example above, though with the following differences:

* The ``purge:`` option (which defaults to `no`) ensure that **only** the specified entries are present. All other entries will be **deleted**.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensures


.. code-block:: yaml

- name: Enable port
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which modules is being used here? This example seems sort of incomplete.

INTERNAL NOTE: Only now that we've explained the problem and given an example should we go into details.


Declarative intent modules are designed to provide playbook designers a set of network modules that perform declarative configuration tasks on network devices. This includes the ability to declaratively describe a configuration set. In addition, declarative intent modules will also provide a means for declaratively expressing the intended ephemeral state of configuration resources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some context:

Previous network modules allowed users to update a device by listing out the steps that need to be taking in order to achieve a desired state. Declarative intent modules......


.. code-block:: yaml

- name: configure interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there should be a module in this task

@samdoran samdoran added networking Network category and removed needs_triage Needs a first human triage before being processed. labels Jul 19, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 26, 2017
@ansibot
Copy link
Contributor

ansibot commented Jul 26, 2017

The test ansible-test sanity --test rstcheck failed with the following error:

docs/docsite/rst/network_declarative_intent.rst:118:0: Title underline too short.

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 26, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 26, 2017
@ansibot
Copy link
Contributor

ansibot commented Jul 26, 2017

The test ansible-test sanity --test rstcheck failed with the following error:

docs/docsite/rst/network_declarative_intent.rst:218:0: Title underline too short.

click here for bot help

Copy link
Contributor

@dharmabumstead dharmabumstead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review comments. Please flesh out the rest of 'declarative_intent' and I will make an edit pass over that as well. Good stuff - thanks @gundalow!

Networking Guides
*****************

This section explores particular Ansible Networking functionality and use cases in greater depth and provide a more "top down" explanation of some basic features.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"provide"->"provides". Also, I'd consider changing "top down explanation" to "an overview of some basic features". We're trying to minimize colloquialisms in the docs that might make future localization work more difficult.


.. contents:: Topics

This section explores how and when `Aggregate Resources` can be used within Ansible Networking.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't capitalize "Aggregate Resources" unless it's a proper name or a trademark. Re: 'Ansible Networking' - in most cases (unless it's a trademark or proper name) 'networking' shouldn't be capitalized.


This section explores how and when `Aggregate Resources` can be used within Ansible Networking.

This document is part of a collection on Networking. The complete list of guides can be found at :ref:`Network Guides <guide_networking>`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'networking'


Consider the case if you wanted to ensure a set of vlans are present on a switch. Although at a high level this sounds like a simple request, there in a slight nuance:

* Should these vlans be in *addition* to the existing configuration? - *additive*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please spell these out. Something like: "If the vlans are to be in addition to the existing configuration, they are considered additive vlans".


.. versionadded:: 2.4

The ``aggregate:`` option has been added in Ansible 2.4 and is available in certain modules, see the modules documentation to see if the feature is available.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semicolon or period and new sentence after "certain modules" please.


.. warning:: Why does ``purge`` default to ``no``?

To prevent from accidental deletion ``purge`` is always set to ``no``. This requires playbook writers to add ``purge: yes`` to enable this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma after deletion.

=======================================

* What
* Why: Cleaner short hand. Allows you to separate out what's common from what's item specific.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write this section out with full sentences. It reads like shorthand notes.

state: active # override
purge: yes # override

Become realy power on ``net_interfaces``, mtu, admin_state, description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

purge: yes


When would you use aggregate resources with ``purge: true``?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ibid.


.. contents:: Topics

This section explores what `Declarative Intent modules` are within the context of Ansible Networking.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revisit the capitalization here, as per comments above.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 29, 2017
- name: configure vlans neighbor (delete others)
net_vlan:
aggregate:
- 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way of specifying value is not supported. Field under aggregate should be a key value pair.
eg: vlan_id: 1


* Should we warn if purge & not aggregate

* Do we want to add ``required_if = [('purge', 'true', ['aggregate'])]``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be always required. If aggregate is absent and purge is True it should delete all configured field under specified hierarchy.

enabled: yes

# Intended state
state: connected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

----------------------

All declaritive intent modules support a ``delay:`` option. This represents the amount of time, in seconds, that Ansible will wait after setting declaritive configuration before checking the indended state. This pause is needed to allow the network device being configured to stablise, such as to allow handshake after bring up an interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value of delay is 10 seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added "The default value is specified in the module's documentation."
Module documentation updated in #30344

@Qalthos Qalthos modified the milestone: 2.4.0 Sep 1, 2017
- { vlan_id: 6 }
name: reserved_vlan # override
state: active # override

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you use aggregate and the normal attribute?

 - name: Reserve mgmt vlans
    net_vlan:
       aggregate:
         - { vlan_id: 4 }
         - { vlan_id: 5, name: mgmt }
         - { vlan_id: 6 }
       name: reserved_vlan # override
       state: active # override
       vlan_id: 5 <--- used in both aggregate and normal attribute 
       name: not-mgmt

If this is not allowed might be good just to note that. Agreed this is not a useful thing to do but someone could do it and it would good to know what the outcome would be

@ansibot
Copy link
Contributor

ansibot commented Sep 6, 2017

@gundalow gundalow changed the title [WIP] Docs network 2.4 [WIP] Docs network 2.4 (aggregate & declarative intent) Sep 11, 2017
@gundalow gundalow changed the title [WIP] Docs network 2.4 (aggregate & declarative intent) [WIP] Docs network 2.4 Declarative intent Sep 12, 2017
@gundalow
Copy link
Contributor Author

Based on feedback in Network Contributors Summit in SF I've removed "aggregate" so in 2.4 we can release it as an "undocumented tech preview", this will allow further discussion to be had in the Community regarding how we want to proceed.

A backup of the original file can be found at https://gist.github.com/gundalow/c2a4fb51a093a72bc4a380535aa1d835

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 12, 2017
@gundalow gundalow modified the milestones: 2.4.0, 2.5.0 Sep 15, 2017
@gundalow
Copy link
Contributor Author

The Core Network team has agreed that we will not add the technical guide for Declarative Intent into 2.4.0 to avoid confusing the end users.

Therefore removing 2.4.0 milestone

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 23, 2017
@victorock
Copy link
Contributor

Rollback should be used carefully hence all changes should be validated as part of Network CI pipelines for Dev/Test/QA before being promoted to production.

@gundalow gundalow removed this from the 2.5.0 milestone Jan 17, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 6, 2018
@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 1, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 31, 2018

@samccann
Copy link
Contributor

@gundalow @ganeshrn Anything we can do to move this one along?

@gundalow gundalow closed this Sep 12, 2018
Ansible-maintained Collections Documentation automation moved this from Docsite work to Done Sep 12, 2018
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 docs This issue/PR relates to or includes documentation. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html networking Network category stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants